fix(runtime): GET /api/v1/packages/:id honours ?version= instead of silently ignoring it - #17668
Conversation
… it (#17416) The route accepted `?version=` from the SDK and the only surface serving it never read the parameter: the caller got `200` with the installed row and nothing in the status, headers or body told that apart from a version-scoped read. The handler that honoured it went with the REST twin in #14503/#16628 and this dispatcher domain never had the read to inherit. Request-side only — the response shape is untouched (#12034 owns it), so the route still answers with exactly one body shape. Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude <noreply@anthropic.com>
) The discriminating pin (status + the field that names the version, and no package row on the refusal) beside the lit control (the unversioned read still answers exactly as it did). A green on only the new path would also pass with the ordinary read broken, and this door is the route's single implementation. Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude <noreply@anthropic.com>
📓 Docs Drift CheckThis PR changes 1 package(s): 18 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 3 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 25 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin c5a9a89f238d811882803059f1422e33b88fd746 && git checkout c5a9a89f238d811882803059f1422e33b88fd746
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e1fdaf8b039a0aa02fc3b3d8ca6166f74cc19ba3 0e89a0d7c5311747e737dce3115fb98bdfab3f00 && git checkout -B drift-repro e1fdaf8b039a0aa02fc3b3d8ca6166f74cc19ba3 && git merge --no-ff 0e89a0d7c5311747e737dce3115fb98bdfab3f00
node scripts/docs-audit/affected-docs.mjs --json e1fdaf8b039a0aa02fc3b3d8ca6166f74cc19ba3
|
|
ACCEPT with one follow-up card — Reviewed off the diff, not off the PR body: ⛔ Not armed on purpose. This PR was a draft, so its CI has not run against the ready state. This seat's standard for enqueue resistance is every check green, not only the required ones — arming before that lets the queue merge on required-only green with an advisory check red, which is shared damage. Ready now so CI runs; armed when the check-run read is clean. What this seat verified independently
One follow-up card, filed: #17672The repeated This is not a blocker and nothing here asks for a change to this PR. The three alternatives inside the declared file surface were all worse, and the PR says so in the code rather than in a commit message: copying the rule is the drift its own header forbids, exporting it widens another package's published surface (a Two things made it a card rather than an acceptance note:
Measured by this seat for #17672, with the control beside it:
On the declared deviations
Landing verification is owed after the queue merges: single-parent squash (
Generated by Claude Code |
|
⛔ CI is RED, and the ACCEPT above was posted without reading it. Correcting that first, then the blocker. — The correction, because it is mineMy ACCEPT ( ⭐ The one thing that held: auto-merge was deliberately not enabled here. The red never had a path to What is failing — one test, this PR's own
A one-millisecond difference in two wall-clock stamps. ⛔ It is not an infrastructure flake, and it is not the base branch's: it is a test this PR adds, failing on a variable this PR's test controls. Which cause, and the control that separates themTwo readings are possible and they take different actions:
⭐ The file already contains the control. §2's "is byte-identical to the read with no query object at all" makes one DispositionThe card's dev agent has been resumed on the same branch and worktree with the failure, the control, and hard limits: ⛔ never skip, disable or quarantine the test; ⛔ no empty commit and no re-run-to-green; ⛔ do not weaken the assertion into a spot check or strip the two timestamp fields out of the comparison — whole-body ⛔ This PR is not armed and will not be until every check on its then-current head reads green. ⭐ The ACCEPT's substantive findings — the premise measurement, the fenced exit correctly not taken, the ablation's on-disk proof,
Generated by Claude Code |
…d row (#17416) CI's Test Core (2/6) failed §3 on a 1 ms timestamp difference. Cause measured: `get()` built a fresh host per call, so the package was installed twice, and `SchemaRegistry.installPackage` stamps `installedAt`/`updatedAt` from one `new Date()` per install. Two hosts therefore hold two rows whose stamps differ whenever the installs straddle a millisecond boundary — a flake, not a product defect: nothing on the read path reads a clock (`toPackageResponse` is an allowlist copy, `withWritableVerdict` a spread, and `success()` adds no timestamp), so one install cannot produce two answers. Every case that compares two response bodies now issues both requests through one host, which is the shape §2's byte-identical pin already had and the reason it stayed green in the same shard. §1's pair moves onto one host too: the criterion is «the SAME request with and without `?version=`», and two hosts let a difference come from the rows instead of from the parameter. §4's array-vs-string pin carried the identical latent flake and is repaired with it. Whole-body `toEqual` is preserved everywhere — the stamps stay in the comparison, which is what makes "the same request" mean the same response. Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude <noreply@anthropic.com>
|
GREEN and ARMED — The CI reading, this time actually takenHead confirmed first (a push moves the sha): ⭐ Every check, not only the required ones — that is this seat's arming standard, and this time it is a reading rather than an assumption. Which cause — settled from the diff, ⛔ not from the greenThe dispatch left two readings open and said they take different actions: (a) the fixture installs twice (a test bug) or (b) the read path re-stamps It is (a), and ⭐ (b) is excluded affirmatively rather than by omission — the new docblock names every step of the read path and shows none of them reads a clock:
⇒ No product finding is owed, and that is a measured conclusion rather than a silence. ⭐ The distinction the docblock draws — "One host makes it deterministic rather than merely likelier" — is exactly what the dispatch asked for: a flake fixed at a millisecond boundary would otherwise pass on a re-run and come back. Three things this repair got right that the failing line did not require
⛔ No test was skipped, disabled or quarantined; no empty commit; no re-run-to-green; nothing outside the declared file surface. ArmedAuto-merge enabled 10:54Z. Landing verification still owed after the queue merges: that parent count plus a controlled content read on
Generated by Claude Code |
…ALIDATION_ERROR from the one shared rule (#17672) The door refused a repeated `?version=` with `404` and a sentence of its own, so a request-shape error was indistinguishable from the two genuine not-founds the same door answers. The repo's one rule for this condition already answers `400 VALIDATION_ERROR` in the ADR-0112 nested body; what blocked #17668 from calling it was that `packages/rest/src/query-multiplicity.ts` is reachable from nowhere outside its package. - `@objectstack/rest`'s barrel publishes `repeatedQueryParamMessage` and `refuseRepeatedQueryParams`, with the entry recording which half is portable across a package boundary and which is not. - The dispatcher's `/packages` domain calls the message function and drops its local copy; `deps.error(msg, 400)` derives `VALIDATION_ERROR`. - The module header's "and it reads no `version`" parenthetical is corrected — false since #17668 landed, and load-bearing prose about why the rule has one home. - `packages-get-version-scope.test.ts` §4's pin is deliberately changed from the interim `404` to the end state, and §5 pins the distinction the card is about. Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude <noreply@anthropic.com>
Fixes #17416
GET /api/v1/packages/:idaccepted a?version=query parameter and the only surface serving the route never read it. A caller asking for a version that is not installed was answered200with the installed row, and nothing in the status, headers or body distinguished that from a version-scoped read that actually happened.The maintainer ruling on the card: honour
?version=, or refuse it so the caller knows — silently ignoring is not a third option. This is the honouring half, and it needed no SDK change.The premise held — honouring was reachable
The card's dispatch named the premise to measure first, and it measures true:
handlePackagesRequestalready receivesqueryand already uses it on sibling branches (query?.status,query?.type,query?.overwrite,query?.keepData), so "the handler cannot see the parameter" was false.packages/runtime/src/domains/packages.ts:1211— unmoved since dispatch, verified against this branch's merge base rather than recalled.@objectstack/service-package'sget(packageId, version)reads it durably withAND version = ?.⇒ the fenced exit (refuse server-side, which would have required
packages/client/src/index.ts, hard-serial behind #17454) was not taken and is not needed.What the route answers now
GET /packages/:id200, installed row200, installed row — unchanged?version=1.0.0(the installed one)200, installed row200, installed row?version=latest200, installed row200, installed row?version=99.0.0(not installed)200, installed row404, naming both versions?version=a&version=b200, installed row404, naming the repetition?version=…on an id that is not installed404 Package 'ID not found'(the existing wording)Answering the dispatch's explicit question — what happens when the requested version does not exist:
404. Never a200carrying the installed version, which is this card's defect one layer in. The message names the requested version and the installed one, so the caller can tell "you do not have this version" from "no such package":Three decisions worth reading:
latestand "no parameter" are one request, deliberately. The deleted handler readrequested.value || 'latest'and its store resolvedlatestto the newest row, so those two named one request there; they name one request here. Continuity with the contract the deleted door published, not a new sentinel invented at this door.packages-single-door.test.tspins, with or without?version=— a package that is not installed cannot be at the wrong version.PackageRecordout of the durable store would have introduced a second body behind a query parameter, and that surface belongs toclient.packagesdeclares envelopes two mounted surfaces do not emit:getdiverges between them, andinstall/enable/disabledeclare{ package }where the only serving surface sends the bare row #12034 / fix(client): bind packages.get to the bare InstalledPackage row on both the global and scoped clients #17419 (correctly fenced off this card).Comparison is exact string equality, the same predicate the durable store uses, so the two answers to "is this package at version v" cannot drift into semver-range semantics at one of them.
Verification
Reverse verification (the fix committed first, the handler then restored to this branch's merge base and the mutation proven on disk before the run):
The direction is turns red, and the three failures print the defect's own signature —
expected 200 to be 404is the200that hid this for the whole of the parameter's life. The six that stay green are the lit control and the two requests that mean "the installed row", which is the control working rather than a gap. Restore proven by blob hash againstHEAD, not by an exit code.Tests —
packages/runtime/src/domains/packages-get-version-scope.test.ts, a realSchemaRegistrybehind the realHttpDispatcher, the way the sibling door tests next door drive it:?version=naming a version that is not installed does not produce the same response.statusplus the discriminating field (the message naming both versions; no package row on the refusal), never "it returned 200".200with the installed row and itswritableverdict, and is byte-identical to the read with no query object at all.latestand absent are one request. §4 a repeated parameter is not resolved silently, and a one-element array is one occurrence.pnpm --filter @objectstack/runtime exec vitest run --project localVERDICT command-exit 0— 255 files / 3583 tests passed, re-run on0e89a0d7pnpm --filter @objectstack/runtime typecheckVERDICT command-exit 0— test-layer debt ledger unmoved (27 files / 191 errors / 69 pinned)pnpm --filter '@objectstack/runtime^...' build --concurrency=2VERDICT command-exit 0pnpm lint(the full union,eslint . --no-inline-config)0e89a0d7The CI red on
ffdc9a5b, and why it cannot come backTest Core (2/6)failed §3 on a 1 ms timestamp difference. Cause measured, not assumed: the fixture installed twice.get()built a fresh host per call, andSchemaRegistry.installPackagestampsinstalledAt/updatedAtfrom onenew Date()per install (packages/objectql/src/registry.ts:4171), both of them declared record fields this door carries to the wire. Two hosts therefore held two rows whose stamps differ whenever the installs straddle a millisecond boundary.The other candidate — the door re-stamping on read — is excluded mechanically: nothing on the read path reads a clock.
toPackageResponseis an allowlist copy,withWritableVerdicta spread,installedVersionOfandreadRequestedVersionare pure, andHttpDispatcher.success()returns{ success, data, meta }with no timestamp. §2's byte-identical pin — one install, two requests, the same whole-bodytoEqual— passed in the same run and the same shard, so the variable separating green from red was the install count, not?version=.Reproduced deterministically before repairing, rather than waiting for the race: injecting a 5 ms separation between §3's two calls turned the 1-in-N flake into a certainty, with the stamp gap equal to the injected delay.
The repair is one install, not a frozen clock and not a narrowed assertion. Every case comparing two response bodies now issues both requests through one host — §2's shape, which is why it was already green. Whole-body
toEqualis preserved everywhere and both stamps stay in the comparison: that is what makes "the same request" mean the same response rather than the same status. §1's pair moved onto one host too, which strengthens the card's own criterion — «the SAME request with and without?version=» is about one row, and two hosts let a difference come from the rows instead of from the parameter. §4's array-vs-string pin carried the identical latent flake and is repaired with it.It cannot drift again because there is no second install and no second clock read: both bodies are projections of ONE row. Demonstrated positively rather than by a passing re-run — injecting a 50 ms separation, ten times what made the old shape fail with certainty, between the repaired §3's two reads leaves all 9 green (
DETERMINISM command-exit 0).Gates — re-derived on the final diff with
node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstackand reconciled with--ran, each command redirected and its$?captured before any pipe:The two are
check:dual-build-cjs-loadsandcheck:type-check-debt, bothPREREQUISITE NOT MET— each needs a whole-repo build closure on disk and says in its own words that this is "NOT a pass: nothing was measured". Neither is a finding, and neither can be moved by this diff: the first reads publishedexports(none change here) and the second refuses to re-measure a ledger it cannot resolve. They are CI's to measure.The four roster gates whose baselines sit under a directory this diff is in — flagged by the derivation as "silence is not evidence in either direction" — were run rather than assumed:
check:route-ledger-census(exit 0; 82 declared, array holds 82 — this diff adds no route, andRouteLedgerEntrycarries no query-parameter field, so nothing there is owed),check:error-code-casing,check:authz-resolver,check:filter-alias-parity, all exit 0.A changeset is owed and written:
@objectstack/runtimeminor.skip-changesetdoes not apply — a published package's wire answer moves.Acceptance notes
Noted here rather than fixed, because each is outside this card's file surface and none is this parameter on this door:
?limit=and?cursor=are declared and never read,?type=is read and never declared #17667 — the/packagesread doors' declared request schemas and their actual query reads diverge in both directions, and this card is one instance of it.ListInstalledPackagesRequestSchemadeclareslimit(with.default(50)),cursorandenabled; the list door reads none of the three, soGET /packages?limit=1answers200with every row and ahasMore: falsethat agrees with it — this card's defect, on the sibling door, with the parameter the repo's own ingress rule names as the sharpest to drop. In the other direction?type=,?overwrite=and?keepData=are read and declared nowhere, andGetInstalledPackageRequestSchemais path-params-only, which makes the?version=this PR honours an undeclared read too. Filed unassigned, no labels; it is a producer-side wire decision per parameter and wants its own ruling, exactly as this card did.refuseUnknownQueryParams,refuseRepeatedQueryParams) live inpackages/restand are not exported from@objectstack/rest's barrel, so no dispatcher domain can call either without widening another package's public surface. That is why the repeated?version=here is answered404naming the repetition rather than the rule's400 VALIDATION_ERROR: the rule has one home and one message by its own header's insistence, and a second copy with a second sentence is the drift that header forbids. The end state is the400. Carrier: whoever converts the/packageslane to a closed set under the Route and surface ownership rule, which is explicit that this happens per lane and never as one sweep.packages/rest/src/query-multiplicity.ts's header says the dispatcher's/packagesdomain "reads noversion". This PR makes that parenthetical false. It is a code comment in a non-exported module, so it is a note rather than a card, and it is one line for the carrier above to correct in the same edit that makes the rule reachable.⛔ Not touched, as fenced by the dispatch:
packages/client/src/index.ts(hard-serial behind #17454 — and not needed, since the SDK's declaration becomes true rather than wrong) andpackages/runtime/src/dispatcher-plugin.ts(held by #17432 this round). No breach.Authored by the
domain:clidev seat in sessionsession_01TSf4DV7ziu4V5j73e46b7c(https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c).Generated by Claude Code